Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

This removes many const char* warnings related with LWIP_ASSERT() #10500

Merged
merged 1 commit into from May 12, 2019
Merged

This removes many const char* warnings related with LWIP_ASSERT() #10500

merged 1 commit into from May 12, 2019

Conversation

andrewc-arm
Copy link
Contributor

@andrewc-arm andrewc-arm commented Apr 27, 2019 •
edited

Description

If we enable LWIP debugging option like "lwip.debug-enabled": true,, we get many warnings related with LWIP_ASSERT like following.

[Warning] cc.h@117,78: deprecated conversion from string constant to 'char*' [-Wwrite-strings]
[Warning] cc.h@117,78: deprecated conversion from string constant to 'char*' [-Wwrite-strings]
[Warning] cc.h@117,78: deprecated conversion from string constant to 'char*' [-Wwrite-strings]

For more information, please refer to the attached build_log.txt file.
build_log.txt

The code change removes duplicate warnings.
This problem was reproduced in GCC_ARM and fixed in GCC_ARM. Other toolchains are not tested.

Pull request type

[x] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

Release Notes

@ciarmcom ciarmcom requested review from a team April 27, 2019 15:00
@ciarmcom
Copy link
Member

@andrewc-arm, thank you for your changes.
@ARMmbed/mbed-os-ipcore @ARMmbed/mbed-os-maintainers please review.

kjbracey
kjbracey approved these changes Apr 29, 2019
View reviewed changes
Copy link
Contributor

@kjbracey kjbracey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine, but I'm curious where you're seeing the warnings. Which toolchain? Have you enabled extra warnings?

The lack of const here would be a problem in C++, where string literals are const char[], but in C they're char[], so passing a string literal to this is valid, and I don't recall seeing any warnings myself.

0xc0170
0xc0170 requested changes Apr 29, 2019
View reviewed changes
Copy link
Contributor

@0xc0170 0xc0170 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing description and PR type in the first comment (PR template) - please fill in the details

As requested, how was this tested - which toolchain produce warnings?

@adbridge
Copy link
Contributor

adbridge commented May 1, 2019

@andrewc-arm please correctly fill in the PR template header. Needs a proper description and pull request type setting

@andrewc-arm
Copy link
Contributor Author

Sorry for the late response. I have been working on Samsung issues lately. I will get back soon.

@andrewc-arm
Copy link
Contributor Author

Hi, @adbridge and @0xc0170
I updated the PR template. Could you please let me what more is needed?

@kjbracey
Copy link
Contributor

kjbracey commented May 2, 2019

My question about where/why you were seeing warnings remains outstanding.

@andrewc-arm
Copy link
Contributor Author

Hi, @kjbracey-arm

My template comment far above has been updated.

If we enable LWIP debugging option like "lwip.debug-enabled": true,, we get many warnings related with LWIP_ASSERT like following.

[Warning] cc.h@117,78: deprecated conversion from string constant to 'char*' [-Wwrite-strings]
[Warning] cc.h@117,78: deprecated conversion from string constant to 'char*' [-Wwrite-strings]
[Warning] cc.h@117,78: deprecated conversion from string constant to 'char*' [-Wwrite-strings]

For more information, please refer to the attached build_log.txt file.
build_log.txt

I thought the log and error messages are good enough. If not, please let me know.

@adbridge
Copy link
Contributor

adbridge commented May 2, 2019

@kjbracey-arm are you happy with this ?

@kjbracey
Copy link
Contributor

kjbracey commented May 2, 2019

Sorry, missed part of your update.

So the issue is your own ping.cpp file? I guess you've taken lwIP's ping.c, and are compiling it as C++?

Okay, that would explain why I've not seen it - that LWIP_ASSERT would have only ever been used from C code - it's your application's attempt to use it from C++ that's generating the warnings.

Yes, fix is fine - makes it safe for C++ use.

@0xc0170
Copy link
Contributor

0xc0170 commented May 2, 2019

I'll schedule CI a bit later today due to 5.12.3 final patches in CI

@andrewc-arm
Copy link
Contributor Author

Hi, @kjbracey-arm

So the issue is your own ping.cpp file? I guess you've taken lwIP's ping.c, and are compiling it as C++?

I am not so sure where the ping.c came from. I got the original ping.cpp file from Samsung's application code which seems to be Apache 2 style license. I improved it for IPv6 in mbed-os-example-ping. Thanks for approval!

@adbridge
Copy link
Contributor

adbridge commented May 7, 2019

CI started

@mbed-ci
Copy link

mbed-ci commented May 7, 2019

Test run: FAILED

Summary: 2 of 11 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_dynamic-memory-usage
  • jenkins-ci/mbed-os-ci_greentea-test

@andrewc-arm
Copy link
Contributor Author

Hi, @adbridge

This is excerpt of the failure.

[1557254704.66][CONN][INF] found KV pair in stream: {{start;1}}, queued...
[1557254704.66][HTST][INF] host test class: '<class 'rtc_reset.RtcResetTest'>'
[1557254704.66][HTST][ERR] something went wrong in event main loop!
[1557254704.66][HTST][INF] ==== Traceback start ====
Traceback (most recent call last):
File "/usr/local/lib/python3.6/dist-packages/mbed_os_tools-0.0.8-py3.6.egg/mbed_os_tools/test/host_tests_runner/host_test_default.py", line 306, in run_test
self.test_supervisor.setup()
File "/builds/ws/mbed-os-ci_greentea-test@7/mbed-os/TESTS/host_tests/rtc_reset.py", line 43, in setup
generator.next()
AttributeError: 'generator' object has no attribute 'next'
[1557254704.66][HTST][INF] ==== Traceback end ====

I think this failure is not caused by my simple code change in LwIP. Or am I missing something?

@adbridge
Copy link
Contributor

adbridge commented May 8, 2019

@andrewc-arm Looks like there were some changes to the CI which broke the test. We will re-run the tests once they have fixed it.

@0xc0170
Copy link
Contributor

0xc0170 commented May 10, 2019

CI restarted

@mbed-ci
Copy link

mbed-ci commented May 10, 2019

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 2
Build artifacts

@0xc0170 0xc0170 merged commit 03cda26 into ARMmbed:master May 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@0xc0170 0xc0170 0xc0170 approved these changes

@SeppoTakalo SeppoTakalo SeppoTakalo approved these changes

@kjbracey kjbracey kjbracey approved these changes

Assignees
No one assigned
Labels
release-version: 5.12.4
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@andrewc-arm @ciarmcom @adbridge @kjbracey @0xc0170 @mbed-ci @SeppoTakalo